I haven't ever seen attributes used to enable codec features like this, at least it doesn't appear to be common practice in the core codecs. More typically this would tick VERSION_CURRENT and something (probably just the rotation seed?) would be encoded as part of field metadata. I don't have a good sense as to why maybe @mikemccand or @benwtrent has a firmer and better reasoned opinion.

Yeah I also reasonate here. The initial version (1st commit) tried to retain it in the sandox module -> then moved it into the main core Codec but it was writing a random seed to the metadata of the field and I had to bump the codec to 105. I honestly didn't wanted to bump the Codec for this given usecase so I switched to use a constant seed and assuming rotation is enabled for all vector fields by default(which obviously simplified all of this but takes away the capability from user to configure it on per field basis). So eventually I biased towards sharing the rotationEnabled flag (configured per vector field) to query time via FieldInfos and avoiding Codec bump since this way we were not breaking the backward compatibility and also how simple it was in nature. I'm open to ideas whichever we would want to choose or if there is a better way to share this info(rotation enabled/disabled) hopefully avoiding the codec bump.

IIUC this rotation operation is probably fairly expensive -- at least as expensive as quantization but possibly even more expensive. In Lucene's segment structure this operation will be repeated for every segment searched. In your tests you probably ran with everything merged down to a single segment but I'm interested in what this costs in a more typical multi-segment setup. A microbenchmark for the rotation or a full luceneutil run would be helpful here.

Depending on the cost we might want to figure out a way to reuse this computation across segments somehow which probably requires upstream integration with the knn query classes.

rotate is a O(d log d) operation here. I ran the luceneutil without forceMerge too but I don't have the Cohere results handy anymore(I can pull those up again). Though I have the results with 4K dim embeddings in multi segment index. Sharing those below for you reference(will share the Cohere one also soon). I didn't seem to regress the latency as such. We can do more JMH benchmarking or whatever could give us more confidence but so far it appears to be a cheap cost(Idk if the quantization overhead was significant in past).

We cannot do a rotation per segment, that is a non starter.

This would significantly impact latency in the most common scenario, which is many segments.

Instead I suggest queries need to have an "additional phase" that looks to see if any of the KnnFormats can apply a "globalPrecondition" step or something and then apply it once for the query.

To see the significance of the performance impact, you will need many vectors spread over many segments (which is very common, I mean 10s of millions spread over 30-50+ segments).

@benwtrent I see, yeah I think moving in upstream and doing it once makes sense to me. Maybe just moving it to KnnQuery as Trevor mentioned below?

To see the significance of the performance impact, you will need many vectors spread over many segments

That could be the case yes. Sharing below the cohere v3 run I put with multi segment and it seems to have not much impact but larger segments of 30M might move the needle.

Results:
NOTE: nDoc = 500000 for all runs; skipping column
NOTE: searchType = KNN for all runs; skipping column
NOTE: topK = 100 for all runs; skipping column
NOTE: fanout = 100 for all runs; skipping column
NOTE: resultSimilarity = N/A for all runs; skipping column
NOTE: decay = N/A for all runs; skipping column
NOTE: resultCount = 100.000 for all runs; skipping column
NOTE: maxConn = 64 for all runs; skipping column
NOTE: beamWidth = 250 for all runs; skipping column
NOTE: force_merge(s) = 0.00 for all runs; skipping column
NOTE: filterStrategy = null for all runs; skipping column
NOTE: filterSelectivity = N/A for all runs; skipping column
NOTE: overSample = 1.000 for all runs; skipping column
NOTE: bp-reorder = false for all runs; skipping column
NOTE: indexType = HNSW for all runs; skipping column
NOTE: rerank = no for all runs; skipping column
recall  latency(ms)  netCPU  avgCpuCount  quantized  visited  index(s)  index_docs/s  num_segments  index_size(MB)  vec_disk(MB)  vec_RAM(MB)
 0.695        1.916   5.285        2.758     1 bits    54581     94.09       5314.12             5         2092.52      2020.836       67.711
 0.817        1.887   6.052        3.208     2 bits    50314     95.73       5223.08             5         2147.02      2081.871      128.746
 0.921        2.107   7.275        3.453     4 bits    50877     97.97       5103.60             6         2266.48      2204.895      251.770
 0.976        3.300  12.843        3.892     7 bits    61241    101.92       4906.00             9         2506.59      2449.036      495.911
 0.983        3.280  12.810        3.905     8 bits    61080     99.65       5017.51             9         2506.62      2449.036      495.911

Results:
NOTE: nDoc = 500000 for all runs; skipping column
NOTE: searchType = KNN for all runs; skipping column
NOTE: topK = 100 for all runs; skipping column
NOTE: fanout = 100 for all runs; skipping column
NOTE: resultSimilarity = N/A for all runs; skipping column
NOTE: decay = N/A for all runs; skipping column
NOTE: resultCount = 100.000 for all runs; skipping column
NOTE: maxConn = 64 for all runs; skipping column
NOTE: beamWidth = 250 for all runs; skipping column
NOTE: force_merge(s) = 0.00 for all runs; skipping column
NOTE: filterStrategy = null for all runs; skipping column
NOTE: filterSelectivity = N/A for all runs; skipping column
NOTE: overSample = 1.000 for all runs; skipping column
NOTE: bp-reorder = false for all runs; skipping column
NOTE: indexType = HNSW for all runs; skipping column
NOTE: rerank = no for all runs; skipping column
recall  latency(ms)  netCPU  avgCpuCount  quantized  visited  index(s)  index_docs/s  num_segments  index_size(MB)  vec_disk(MB)  vec_RAM(MB)
 0.729        1.845   5.056        2.740     1 bits    51755     96.87       5161.40             5         2090.45      2020.836       67.711
 0.843        1.870   5.904        3.157     2 bits    49123     98.38       5082.18             5         2146.54      2081.871      128.746
 0.940        2.089   7.143        3.419     4 bits    50584     95.99       5209.04             6         2266.39      2204.895      251.770
 0.988        3.255  12.872        3.955     7 bits    60998    100.92       4954.52             9         2506.59      2449.036      495.911
 0.993        3.244  12.887        3.973     8 bits    60955    100.85       4958.01             9         2506.60      2449.036      495.911

I ran the luceneutil without forceMerge too but I don't have the Cohere results handy anymore(I can pull those up again).

Note that knnPerfTest.py in luceneutil now enables autologger so all output from every run should be in your $LOGS_DIR. I added autologger for exactly this situation!! I got so annoyed losing track of my runs...

rotate is a O(d log d) operation here.

Wait, this is not so costly? It's like log(d) dot products for each segment, when we do many dot products per vector per (large-ish) segment. It's fixed cost per segment, and amortizes to zero as the index gets bigger.

To see the significance of the performance impact, you will need many vectors spread over many segments (which is very common, I mean 10s of millions spread over 30-50+ segments).

Actually, this should be the best case scenario -- the larger the index/segment, the more dot products you'll need to crawl its graph, reducing this small added fixed per-segment cost of log(d) dot products?

I don't really understand the fuss. The added cost (amortizes to zero) seems reasonable if we really do see such recall lift.

@shubhamvishu you could create a tiny tool in luceneutil to just do a one-time rotation of .vec file? Then we all could quickly evaluate impact on our own vector corpora, even without this PR, for starters.

But anyway +1 to make a single global rotation matrix for each vector field, not because I'm worried about the performance impact of per-segment query vector rotation, but for a different reason: because I think this would then unlock data blind quantization as long as vectors are L2 unit-sphere normalized. Then (once your vectors are truly isotropic), optimal quantization is solely a function of int D (dimension) and not the actual vectors, which would be amazing:

• Faster indexing since we wouldn't need the extra pass through the vectors to get histogram of values across all dimensions / centroid for the segment (to tune the quantizer, which is not data blind today)
• Merging could then bulk-copy vectors in quantized form, since their quantized encoding will not change
• We could fully drop full precision vectors (!!) since they are no longer needed for tuning quantizer. Of course user should still have option to keep them, e.g. maybe they want to re-rank.

But let's keep this PR focused on pre-conditioning! Data blind quantizer, options to keep full precision vectors or not, can come later...

I addressed this piece and other comments(of having single rotation matrix per dim). We now rotate the incoming query only once in the query rewrite(KnnFloatVectorQuery#rewrite) itself (no more per segment overhead) and same with rotation matrices.

you could create a tiny tool in luceneutil to just do a one-time rotation of .vec file?

@mikemccand similar to having rotation in luceneutil, now in the latest revision of this PR the rotation has a new specific knn vector format(takes another knn vector format as input) which completely decouples rotation feature.

I don't know about this. This isn't cheap. We have "fieldnum_segmentssize(HadamardRotationObject)",

Seems to me the rotation matrix should just be stored off heap :/ Or the rotation is by dimension, not by field name (idk why we need a new random matrix for this for two fields that have the same dimension).

idk why we need a new random matrix for this for two fields that have the same dimension

It was because the random seed is taking the field name into picture but I agree to your point we could reuse the same matrix across vectors of same dimension actually (likely there is no/much benefit of having random seeds other than avoiding the possibility of choosing a bad seed for all vector fields but this simplification overshadows that possibility without completely discarding). I'll try to stick to a single rotation matrix for a dimension only. Do you think there is enough value of moving it off heap after having 1 rotation or it'll be an overkill?

I made it a single global matrix (per dimension) and not maintaining it per segment now. Thanks!

Instead of adding rotation code to a specific format, can we create a new vector format that simply rotates the incoming vector in all functions, and passes it on to a delegate?

This would cleanly separate rotation functionality + make it re-usable across any arbitrary vector format.

This is a really cool approach @kaivalnp! Then any KnnVectorsFormat out in the wild could quickly/easily insert preconditioning. Maybe look at existing delegating formats (PerFieldXXX)?

+1 I too love the idea @kaivalnp. I like the part of any knn vector format should be able to integrate rotation without extra wiring or duplication. I added a RotationAwareKnnVectorsFormat which pretty much does this now (rotated the vectors when indexing and provides rotated/unrotated views of it at read time accordingly). I confirmed with a luceneutil the recall improvement still holds confirming it works as expected(similar to earlier approach).

I'm not a fan of this pattern -- the responsibility of processing the query vector should lie with the format.

This leads to a possibility of vector queries producing garbage results if they do not consume this attribute correctly (for example, FloatVectorSimilarityQuery was missed in this PR).

Perhaps one way to catch such cases is to randomly wrap the KNN format being tested with a rotating one in BaseKnnVectorsFormatTestCase (or if we can accept slower tests: explicitly test everything with and without rotation).

That said, readers / writers in KnnVectorsFormat are segment-specific and not suitable to process rotation here, perhaps we need a global pre-processing step in the format itself?

I haven't thought too deeply about what the API would look like though.

perhaps we need a global pre-processing step in the format itself?

This is difficult, every segment could have a different format version. There isn't really an idea of a "global format for all the segments".

I think it is possible to simply provide hints that preprocessing has occurred. For example, first iterate all segments, preprocessing the query and this provides some metadata on the query indicating that its been appropriately pre-processed. Then when its passed during the query, that metadata can be checked for. If its there, huzzah! Processed! Otherwise, go the slow path and preprocess.

I am not 100% sure this type of API will work either :/

@kaivalnp @benwtrent I agree with the concerns here and the challenge of such complicated API. I'm thinking if for starter we should keep the rotation per segment and keep this change restricted to rotation like a recall vs latency tradeoff. We could follow with more ideas on how we could clawback the latency. One idea would be to try and see if SIMD could help if some parts of rotation logic or we could think of the sort of API as mentioned which could do this. But doing this in this PR I feel would make this more complicated(or errorprone since we don't have a clear vision for such an API yet). Another good thing of doing it per segment would be that way the whole rotation logic would be retained in the KNN vector format and not spilling into Query implementations etc so we won't be unintentionally breaking any current use cases but providing user an way to improve recall at the cost of some latency overhead for starters and later optimize (keeping this change focussed on capability addition).

I also ran JMH benchmarks(below) on Graviton 3 to see how many dot products(byte vectors) a single rotation operation is equivalent to in some scenarios (it would change with machine vs machine or different quantization etc scenarios). When compared to byte vectors dot product (using Panama based vectorization), a single rotation operation is roughly equivalent to ~50-60 dot products max and when using optimized native kernels it is roughly equivalent to ~200-350 dot products max. This extra rotation overhead might be (or maybe not) within the range we could swallow but looking for suggestions if this overhead seems larger than the potential overhead we initially thought or less than that? (at least for some specific use cases [dim, segment size] the overhead might be less though we could easily think of a situation where the overhead could be large like with 1000 visited nodes(dot products) visited per segment 300 could be <= 20-30% overhead roughly but with large segments it becomes less dominant and even with Panama when compared to other faster ways).

Graviton 3

Benchmark                                   (size)   Mode  Cnt    Score    Error   Units
HadamardRotationBenchmark.inverseRotate        128  thrpt   15    1.493 ±  0.024  ops/us
HadamardRotationBenchmark.inverseRotate        256  thrpt   15    0.708 ±  0.013  ops/us
HadamardRotationBenchmark.inverseRotate        702  thrpt   15    0.257 ±  0.002  ops/us
HadamardRotationBenchmark.inverseRotate       1024  thrpt   15    0.160 ±  0.001  ops/us
HadamardRotationBenchmark.inverseRotate       4096  thrpt   15    0.035 ±  0.001  ops/us
HadamardRotationBenchmark.rotate               128  thrpt   15    1.470 ±  0.004  ops/us
HadamardRotationBenchmark.rotate               256  thrpt   15    0.692 ±  0.010  ops/us
HadamardRotationBenchmark.rotate               702  thrpt   15    0.246 ±  0.001  ops/us
HadamardRotationBenchmark.rotate              1024  thrpt   15    0.155 ±  0.001  ops/us
HadamardRotationBenchmark.rotate              4096  thrpt   15    0.034 ±  0.001  ops/us
VectorUtilBenchmark.binaryDotProductScalar     128  thrpt   15   17.796 ±  0.139  ops/us
VectorUtilBenchmark.binaryDotProductScalar     256  thrpt   15    9.014 ±  0.032  ops/us
VectorUtilBenchmark.binaryDotProductScalar     702  thrpt   15    3.362 ±  0.017  ops/us
VectorUtilBenchmark.binaryDotProductScalar    1024  thrpt   15    2.285 ±  0.021  ops/us
VectorUtilBenchmark.binaryDotProductScalar    4096  thrpt   15    0.566 ±  0.004  ops/us
VectorUtilBenchmark.binaryDotProductVector     128  thrpt   15   60.224 ±  0.010  ops/us
VectorUtilBenchmark.binaryDotProductVector     256  thrpt   15   31.283 ±  0.004  ops/us
VectorUtilBenchmark.binaryDotProductVector     702  thrpt   15   11.609 ±  0.005  ops/us
VectorUtilBenchmark.binaryDotProductVector    1024  thrpt   15    8.039 ±  0.001  ops/us
VectorUtilBenchmark.binaryDotProductVector    4096  thrpt   15    2.024 ±  0.001  ops/us
VectorUtilBenchmark.dot8sNative                128  thrpt   15  135.797 ±  2.069  ops/us
VectorUtilBenchmark.dot8sNative                256  thrpt   15   90.647 ±  3.913  ops/us
VectorUtilBenchmark.dot8sNative                702  thrpt   15   49.021 ±  3.444  ops/us
VectorUtilBenchmark.dot8sNative               1024  thrpt   15   42.404 ±  0.713  ops/us
VectorUtilBenchmark.dot8sNative               4096  thrpt   15   12.222 ±  0.227  ops/us

I'm thinking if for starter we should keep the rotation per segment and keep this change restricted to rotation like a recall vs latency tradeoff.

+1 for PnP ("progress not perfection"). New features should rarely be blocked because of performance, when they are opt-in (not the default) -- that can always be iteratively improved. Still, it's always good to debate approaches / APIs. We should anyways mark the new APIs @lucene.experimental so we are free to iterate them even across Lucene releases.

We could follow with more ideas on how we could clawback the latency.

I'm still confused -- why is latency a problem? It amortizes to 0% as the index gets bigger and bigger? (Because it'd be a fixed per-segment cost, like the cost of seeking the terms dict to locate postings for that segment). Also, "matrix times vector" is such an insanely well optimized function these days... surely we could (eventually) SIMD-ify it? If I read the JMH results right, it's ~6.5 microseconds per rotate for 1K vector, say times 30 segments, 200 microseconds (0.2 msec). That's fine for starters -- it's opt-in -- optimize later!

Another good thing of doing it per segment would be that way the whole rotation logic would be retained in the KNN vector format and not spilling into Query implementations etc so we won't be unintentionally breaking any current use cases but providing user an way to improve recall at the cost of some latency overhead for starters and later optimize (keeping this change focussed on capability addition).

+1 for that simplicity.

Perhaps one way to catch such cases is to randomly wrap the KNN format being tested with a rotating one in BaseKnnVectorsFormatTestCase (or if we can accept slower tests: explicitly test everything with and without rotation).

Big +1 for that -- random testing is powerful, given enough time all bugs are shallow! (The monkey Shakespeare typewriter thing, remixed with Linus' Law heh)

It amortizes to 0% as the index gets bigger and bigger?

It does amortize, but the constant overhead is significant for single bit quantized, which I expect to be even faster than native int8 (which is almost 400x better throughput than the rotation?!?).

I agree, we should aim for opt in and simplicity. I am simply saying we shouldn't incur significant constant overheads when they are avoidable. I am fine with iterating on the opt-in API as it stands now. I have just been pushing a little bit to see if we can land on an API that doesn't have a significantly adverse segments with less than 1M vectors.

I pushed a new revision moving the complete rotation specific logic into the new vector format and keeping it all simple and does not change anything with existing query impl or code paths. Enabling rotation simply means wrapping the any knn vector format with the new RotationAwareKnnVectorsFormat(<some knn format>) and added randomized tests testing it. Also confirmed the earlier rotation benchmark numbers remain the same (so functionally all same)

@shubhamvishu thank you a bunch for iterating on the API and the discussion here :)

Maybe this.delegate = Objects.requireNonNull(delegate)?

Maybe add link to the next constructor? "For indexing, use {@link ...}"?

In fact, Lucene users should never directly use this constructor (since SPI creates all instances at read-time)? Maybe strengthen it to:

    /** No-arg constructor for read-only mode, only for SPI registration.  Do not use this for indexing.  Use {@link...} instead.

Hmm I think this won't work if I have two fields, one with this cool rotating wrapper around vector format A, and then another rotating wrapper around vector format B? Because we are globally going through all fields in this loop, breaking on the first delegate field we see. Maybe see how PerFieldXXXFormat does it? They have the same challenge... they may persist their own mapping file, or maybe they have a simple way to use FieldInfo.

Maybe RotationAwareKnnVectorsFormat_Enabled or so? (So people seeing this FieldInfo attr will know it comes from this wrapper format?

Actually maybe RotationAwareKnnVectors* -> RotatingKnnVectors* everywhere?

Maybe RotationAwareKnnVectorsFormat_DelegateFormat or so?

I asked Claude (Fable 5 -- free for two weeks, and then pay-to-play (by usage)!!) to review -- it had some great items: https://claude.ai/share/bc62ac58-9b15-454e-b52c-7b658babb062 some of which are overlapping my comments, some not!

Actually, I'm confused, why wouldn't it be enabled? Shouldn't it always be enabled? I.e., only use this rotating wrapper format for fields you want to rotate?

Is it because of the byte[] case? Can we just throw exc if user tries to rotate those, for now? Add // TODO saying we may someday add rotation for that case too (no reason it shouldn't work?)?.

Same danger as other merging place (hmm, why are there two?) -- we might end up mixing rotated / non-rotated vectors together?

Hmm I don't like this null check / leniency -- caller should not pass null, that's weird.

Can we take maybe out of the name? Only call this method if you want to rotate.

Throw exception if info is null?